-
Notifications
You must be signed in to change notification settings - Fork 131
Ordered compaction and inlining with full catalog integration #642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Ordered compaction and inlining with full catalog integration #642
Conversation
Basically, we should check that the alter statement does not increase the schema counter of a table, since the alter statement here does not change the actual schema, and the sorting is somewhat "optional" I believe that files should still be compactable before/after the alter.
I wanted to see if files would always be generated with the latest sorting (e.g., by flushing) , and also compacted with the latest sorting, but now that I think about it again, maybe is an overkill because you already have similar tests to that, let's skip it.
Let's skip this. |
|
When investigating 3, I found that you are correct that I am currently incrementing the schema_version when altering the sort order. Dang! I think there are few other cases where we probably want to have the same "alter table, but don't change the schema_version". I'm thinking:
I'll start trying to implement that behavior, but let me know if I need to course correct! From my first prototype, it requires some additional complexity in a few spots (I think we will need a different construct than |
Yes, I think none of those should impact the schema ranges for compaction. I think the main thing you have to do is to ensure that these statements won't write a new entry into |
|
Yup, I will prevent that! Unfortunately, I'm also finding that I need to update the local schema cache (since usually a new schema version would be handled differently). I'm making progress! |
…chema changes and keep schema cache up to date.
|
I now have the schema_version not incrementing on I split the tests apart and also added tests for 1, 2, 3, and 5! I added quite a lot of tests to ensure transactionality for 5. Since I also added tests for the schema_version non-incrementing for the comment cases. Let me know what you think! |
|
So, to fix the assertion issues on my fork's CI, I had to relax an assertion. Please let me know if I am off base, but I think that the assertion was too tight. In src/storage/ducklake_catalog.cpp lines 439-461, the |
|
As I am thinking more about this, I'm wondering if the updates I made to the catalog cache would be safe across processes. Is it safe to not update the schema_version? Could other processes use a stale sort order, or reset it back to being unsorted? |
Could you point me out to which test broke this requirement? From what I can tell, there are other parts of the code that require this to return a table, as we deference it to a e.g., |
Maybe this is something we can test with concurrentloops? (see |
Sure! The tests that broke are here in this CI run on my fork. They were all running a query like COMMENT ON VIEW ducklake.comment_view IS 'con1';I can create a view-specific GetEntryById function if that would be better! |
Unfortunately, I believe that |
Could we achieve this with multiple connections then? Because that's also possible within sqltests |
I am not sure! If you have a spot where I can find an example, I can give it a shot. To understand the behavior, I made a Python script that kicks off 2 separate CLI processes. I found that the sort is ignored by the other process if the schema was already cached ahead of time. The good news is that the catalog DB itself continues to have the right values, but the cache does not get invalidated correctly. The flow is:
If I omit the What do you recommend I do? I've thought about 3 options, but 3 would need some help!
ducklake_set_sorted_multiprocess_add_column.py The logs get printed out for process 1 before printing out process 2, but I logged out some timestamps to show the true order: uv run ./test/ducklake_set_sorted_multiprocess_add_column.py
┌─────────┬──────────────────────┐
│ process │ sum("range") │
│ varchar │ int128 │
├─────────┼──────────────────────┤
│ sql_1 │ 12499999997500000000 │
└─────────┴──────────────────────┘
┌─────────┬─────────────────────────────────────────────────────────────┐
│ process │ (CAST(now() AS VARCHAR) || ' sql_1 finished SET SORTED BY') │
│ varchar │ varchar │
├─────────┼─────────────────────────────────────────────────────────────┤
│ sql_1 │ 2026-01-19 19:16:39.22008-07 sql_1 finished SET SORTED BY │
└─────────┴─────────────────────────────────────────────────────────────┘
┌─────────┬─────────────────────┐
│ process │ sum("range") │
│ varchar │ int128 │
├─────────┼─────────────────────┤
│ sql_2 │ 1999999999000000000 │
└─────────┴─────────────────────┘
┌─────────┬────────────────────────────────────────────────────────────────────┐
│ process │ (CAST(now() AS VARCHAR) || ' sql_2 finished adding column') │
│ varchar │ varchar │
├─────────┼────────────────────────────────────────────────────────────────────┤
│ sql_2 │ 2026-01-19 19:16:35.416129-07 sql_2 finished adding column │
└─────────┴────────────────────────────────────────────────────────────────────┘
┌─────────┬──────────────────────┐
│ process │ sum("range") │
│ varchar │ int128 │
├─────────┼──────────────────────┤
│ sql_2 │ 40499999995500000000 │
└─────────┴──────────────────────┘
┌─────────┬───────────────────────────────────────────────────────┐
│ process │ (CAST(now() AS VARCHAR) || ' sql_2 about to compact') │
│ varchar │ varchar │
├─────────┼───────────────────────────────────────────────────────┤
│ sql_2 │ 2026-01-19 19:16:46.373997-07 sql_2 about to compact │
└─────────┴───────────────────────────────────────────────────────┘
┌─────────┐
│ Success │
│ boolean │
├─────────┤
│ 0 rows │
└─────────┘
┌─────────┬───────────┬────────────┬────────────┬─────────┐
│ process │ unique_id │ sort_key_1 │ sort_key_2 │ bonus │
│ varchar │ int64 │ int64 │ varchar │ varchar │
├─────────┼───────────┼────────────┼────────────┼─────────┤
│ sql_2 │ 3 │ 1 │ woot3 │ NULL │
│ sql_2 │ 2 │ 0 │ woot2 │ NULL │
│ sql_2 │ 1 │ 1 │ woot1 │ NULL │
│ sql_2 │ 0 │ 0 │ woot0 │ NULL │
│ sql_2 │ 7 │ 1 │ woot7 │ NULL │
│ sql_2 │ 6 │ 0 │ woot6 │ NULL │
│ sql_2 │ 5 │ 1 │ woot5 │ NULL │
│ sql_2 │ 4 │ 0 │ woot4 │ NULL │
└─────────┴───────────┴────────────┴────────────┴─────────┘
┌─────────┬─────────────┬────────────────┬────────────────────────────────────────────┐
│ process │ snapshot_id │ schema_version │ changes │
│ varchar │ int64 │ int64 │ map(varchar, varchar[]) │
├─────────┼─────────────┼────────────────┼────────────────────────────────────────────┤
│ sql_2 │ 0 │ 0 │ {schemas_created=[main]} │
│ sql_2 │ 1 │ 1 │ {tables_created=[main.sort_on_compaction]} │
│ sql_2 │ 2 │ 1 │ {tables_inserted_into=[1]} │
│ sql_2 │ 3 │ 1 │ {tables_inserted_into=[1]} │
│ sql_2 │ 4 │ 2 │ {tables_altered=[1]} │
│ sql_2 │ 5 │ 2 │ {tables_altered=[1]} │
│ sql_2 │ 6 │ 2 │ {} │
└─────────┴─────────────┴────────────────┴────────────────────────────────────────────┘
┌─────────┬──────────┬────────────────┬──────────────┬────────────────┬────────────┬────────────────┬────────────┐
│ process │ table_id │ begin_snapshot │ end_snapshot │ sort_key_index │ expression │ sort_direction │ null_order │
│ varchar │ int64 │ int64 │ int64 │ int64 │ varchar │ varchar │ varchar │
├─────────┼──────────┼────────────────┼──────────────┼────────────────┼────────────┼────────────────┼────────────┤
│ sql_2 │ 1 │ 5 │ NULL │ 0 │ sort_key_1 │ DESC │ NULLS_LAST │
│ sql_2 │ 1 │ 5 │ NULL │ 1 │ sort_key_2 │ DESC │ NULLS_LAST │
└─────────┴──────────┴────────────────┴──────────────┴────────────────┴────────────┴────────────────┴────────────┘
|
Hi folks!
This is a new PR meant to solve the same use case as #593, but addressing the PR feedback! Thank you for the guidance - it was super helpful. I am still open to any changes you recommend!
This adds 2 new tables to the DuckLake spec:
ducklake_sort_infoandducklake_sort_expression.ducklake_sort_infokeeps a version history of the sort settings for tables over time. It has 1 row per time that a table has a new sort setting applied. (The prior PR used an option for this, but that has been removed based on the feedback).ducklake_sort_expressiontracks the details of that sort. For each time a new sort setting is applied, this table includes one row for each expression in the order by. (If I order bycolumn3 asc, column42 desc, column64 asc, then there will be 3 rows.)There are still a few limitations with this PR:
expression, so the intention was to make the spec itself forwards-compatible with expression-oriented sorting.I believe that I made this fully compatible with the batching code, but I was testing locally on a DuckDB catalog and not on Postgres. Any extra eyes on that side would be great!
If this looks good, I can also do any docs PRs that you recommend - happy to help there.
Thanks folks! CC @philippmd as well as an FYI